package.json (1)

51-51: Cross-platform consideration for the Xdebug extension path.

The script uses xdebug.so which is Linux/macOS-specific. On Windows, the extension would be xdebug.dll. This should work fine for CI (typically Ubuntu-based) but may require adjustment for local development on Windows.

If cross-platform local development is a concern, consider documenting this limitation or using a shell script that detects the OS.

tests/WP_Ultimo/Managers/Manager_Test_Trait.php (1)

99-112: Consider using markTestSkipped() for clarity.

Using $this->assertTrue(true, ...) to indicate "not applicable" works but is semantically unclear. PHPUnit's $this->markTestSkipped() provides clearer intent and better test reporting.

Proposed improvement

 	public function test_slug_property(): void {

 		$expected = $this->get_expected_slug();

 		if (null === $expected) {
-			$this->assertTrue(true, 'Manager does not define a slug.');
+			$this->markTestSkipped('Manager does not define a slug.');
 			return;
 		}

Apply the same pattern to test_model_class_property().

tests/WP_Ultimo/Managers/Event_Manager_Test.php (1)

96-106: Clean up actions to prevent cross-test leakage.

The anonymous actions remain registered after the test, which can affect later tests that fire wu_event. Consider removing them after assertions.

🧪 Suggested cleanup

-		add_action('wu_event', function () use (&$generic_fired) {
+		$generic_cb = function () use (&$generic_fired) {
 			$generic_fired = true;
-		});
+		};
+		add_action('wu_event', $generic_cb);
 
-		add_action('wu_event_test_fire', function () use (&$specific_fired) {
+		$specific_cb = function () use (&$specific_fired) {
 			$specific_fired = true;
-		});
+		};
+		add_action('wu_event_test_fire', $specific_cb);
@@
 		$manager->do_event('test_fire', ['sample' => 'data']);
 
 		$this->assertTrue($generic_fired, 'wu_event action should have fired.');
 		$this->assertTrue($specific_fired, 'wu_event_test_fire action should have fired.');
+
+		remove_action('wu_event', $generic_cb);
+		remove_action('wu_event_test_fire', $specific_cb);

tests/WP_Ultimo/Managers/Customer_Manager_Test.php (1)

57-65: Consider using $this->expectNotToPerformAssertions() or adding meaningful assertions.

Using $this->assertTrue(true) is a weak pattern that only proves no exception was thrown. For clearer intent, either:

1. Call $this->expectNotToPerformAssertions() at the start of the test if you genuinely only want to verify no exception occurs.
2. Add a meaningful assertion (e.g., verify method returns null or void).

This same pattern appears at lines 77 and 103.

💡 Suggested improvement

 	public function test_log_ip_and_last_login_with_nonexistent_user(): void {
+		$this->expectNotToPerformAssertions();

 		$manager = $this->get_manager_instance();

 		// Should not throw — just return early.
 		$manager->log_ip_and_last_login('nonexistent_user_xyz', null);
-
-		$this->assertTrue(true);
 	}

tests/WP_Ultimo/Managers/Site_Manager_Test.php (2)

81-86: Fix array double arrow alignment.

Static analysis flagged inconsistent spacing around => operators.

🔧 Proposed fix

 		$input = [
-			'good_key'  => 'value1',
-			''          => 'value2',
-			'another'   => 'value3',
-			false       => 'value4',
+			'good_key' => 'value1',
+			''         => 'value2',
+			'another'  => 'value3',
+			false      => 'value4',
 		];

---

141-156: Consider using setUp/tearDown for user context management.

The test manually restores user context with wp_set_current_user(0). For better isolation and to prevent test pollution if the test fails early, consider using class-level setup/teardown.

💡 Alternative approach

private int $original_user_id;

public function set_up(): void {
    parent::set_up();
    $this->original_user_id = get_current_user_id();
}

public function tear_down(): void {
    wp_set_current_user($this->original_user_id);
    parent::tear_down();
}

tests/WP_Ultimo/Helpers/Arr_Test.php (1)

102-109: Consider also asserting the reference modification.

Since Arr::set() takes $array_to_modify by reference, the test should verify that $array itself was modified, not just the return value.

Suggested improvement

 	public function test_set_null_key_replaces_array(): void {

 		$array = ['old' => 'data'];

 		$result = Arr::set($array, null, 'new_value');

 		$this->assertEquals('new_value', $result);
+		$this->assertEquals('new_value', $array);
 	}

tests/WP_Ultimo/Checkout/Line_Item_Test.php (1)

533-540: Use wp_json_encode() for WordPress compatibility (Line 537).

json_encode() is discouraged in WP contexts and can miss WP-specific flags/filters.

✅ Suggested change

-		$json = json_encode($line_item);
+		$json = wp_json_encode($line_item);

tests/WP_Ultimo/Checkout/Cart_Test.php (1)

1666-1669: Drop unused $rate in foreach (Line 1668).

This variable is unused and flagged by static analysis.

✅ Suggested change

-		foreach ($breakthrough as $rate => $total) {
+		foreach ($breakthrough as $total) {
 			$this->assertEquals(0.0, $total);
 		}

tests/WP_Ultimo/Helpers/WP_Config_Test.php (1)

156-188: Strengthen assertions to verify expected line indices.

These two tests only assert that the result is an integer, which is a weak assertion. The tests would pass even if the method returned an incorrect line number. Consider asserting the specific expected values.

♻️ Proposed fix to add specific value assertions

 public function test_find_reference_hook_line_finds_happy_publishing(): void {
 
 	$config = [
 		"<?php\n",
 		"define( 'DB_NAME', 'wordpress' );\n",
 		"/* That's all, stop editing! Happy publishing. */\n",
 		"require_once ABSPATH . 'wp-settings.php';\n",
 	];
 
 	$result = $this->wp_config->find_reference_hook_line($config);
 
-	// The Happy Publishing pattern uses -2 offset
 	$this->assertIsInt($result);
+	// The Happy Publishing comment is at index 2, with -2 offset should return 0 or 1
+	// Verify the expected value based on actual method behavior
+	$this->assertEquals(0, $result); // Adjust expected value as needed
 }
 
 public function test_find_reference_hook_line_finds_php_tag_fallback(): void {
 
 	$config = [
 		"<?php\n",
 		"// Some custom config\n",
 	];
 
 	$result = $this->wp_config->find_reference_hook_line($config);
 
 	$this->assertIsInt($result);
+	// PHP tag is at index 0, verify expected return value
+	$this->assertEquals(0, $result); // Adjust expected value as needed
 }

Note: Verify the expected values against the actual find_reference_hook_line implementation to ensure the assertions match the intended behavior.

tests/WP_Ultimo/Models/Customer_Test.php (4)

330-339: Incomplete comment and redundant method call.

Line 332-333 contains an incomplete comment: "Test that verification key was generated by c" appears truncated. Additionally, disable_verification_key() is called twice (lines 334 and 337), which is redundant since the first call already disables the key.

✏️ Proposed fix

 		$this->assertGreaterThanOrEqual(Hash::LENGTH, strlen($customer->get_verification_key()));
 
-		// Test that verification key was generated by c
-		// Test disabling verification key
+		// Test disabling verification key
 		$disable_result = $customer->disable_verification_key();
 		$this->assertTrue((bool) $disable_result);
-		// Just check that the key was disabled
-		$customer->disable_verification_key();
 		$this->assertEmpty($customer->get_verification_key());

---

856-865: Boundary test may be timing-sensitive.

The test at exactly 3 minutes (strtotime('-3 minutes')) is sensitive to execution timing. If test execution introduces any delay between setting last_login and calling is_online(), the actual elapsed time could exceed the boundary threshold, causing intermittent failures.

Consider using a timestamp slightly under the boundary (e.g., -2 minutes 59 seconds) or mocking the current time to avoid flakiness in slow CI environments.

---

1022-1037: Consider documenting the direct property assignment.

Line 1030 directly assigns to $customer->_user, which appears to be an internal property (underscore prefix convention). While this tests an important scenario where user data is injected (likely from a database JOIN), direct property access is fragile and may break if the implementation changes.

Consider adding a brief comment explaining why direct property assignment is necessary here, or if the Customer class has a public method for this scenario, prefer using that instead.

---

1646-1656: Consider using assertFalse() for boolean assertion.

Line 1655 uses assertEmpty($result) to check has_trialed(). While this works (since false is considered "empty"), using assertFalse() would be more explicit about the expected boolean return type and consistent with the companion test test_set_has_trialed_stores_in_meta which uses assertTrue().

✏️ Suggested change

 		$result = $customer->has_trialed();
 
-		$this->assertEmpty($result);
+		$this->assertFalse($result);

tests/WP_Ultimo/Models/Membership_Test.php (1)

1337-1339: Consider using Membership_Status::ACTIVE for consistency.

The rest of the test file uses Membership_Status constants for status values. Using the string 'active' here is inconsistent and could mask issues if the constant value ever changes.

Proposed fix

-        $result = $this->membership->renew(true, 'active');
+        $result = $this->membership->renew(true, Membership_Status::ACTIVE);
         $this->assertTrue($result);
-        $this->assertSame('active', $this->membership->get_status());
+        $this->assertSame(Membership_Status::ACTIVE, $this->membership->get_status());

tests/WP_Ultimo/Models/Email_Test.php (3)

222-224: Consider removing the section comment.

This comment block adds visual noise to the test file. Test method names should be self-documenting, and IDEs/test runners organize tests by class/method anyway.

---

236-249: Extract repeated reflection logic into a helper method.

The reflection pattern for accessing protected properties is duplicated 20+ times throughout this test file. Consider extracting a helper method to improve maintainability.

♻️ Proposed helper method

/**
 * Get the value of a protected/private property via reflection.
 *
 * `@param` object $object       The object instance.
 * `@param` string $property_name The property name.
 * `@return` mixed The property value.
 */
protected function get_protected_property($object, string $property_name) {
    $reflection = new \ReflectionClass($object);
    $property   = $reflection->getProperty($property_name);

    if (PHP_VERSION_ID < 80100) {
        $property->setAccessible(true);
    }

    return $property->getValue($object);
}

Then usages become:

-$reflection = new \ReflectionClass($this->email);
-$property   = $reflection->getProperty('query_class');
-
-if (PHP_VERSION_ID < 80100) {
-    $property->setAccessible(true);
-}
-
-$this->assertEquals(
-    \WP_Ultimo\Database\Emails\Email_Query::class,
-    $property->getValue($this->email),
-    'Query class should be Email_Query.'
-);
+$this->assertEquals(
+    \WP_Ultimo\Database\Emails\Email_Query::class,
+    $this->get_protected_property($this->email, 'query_class'),
+    'Query class should be Email_Query.'
+);

---

686-701: Consider verifying all provided constructor data.

The test data includes content but there's no assertion verifying it was set correctly. For completeness, consider adding:

💡 Suggested addition

 $this->assertEquals('Constructor Email', $email->get_title());
+$this->assertStringContainsString('Constructor content', strip_tags($email->get_content()));
 $this->assertEquals('system_email', $email->get_type());

tests/WP_Ultimo/Managers/Membership_Manager_Test.php (2)

86-99: Consider using tearDownAfterClass() or ensure test isolation.

The tearDown() method deletes test entities, but if a test fails mid-execution, orphaned entities may remain. This is acceptable for unit tests, but consider whether these entities might interfere with other tests in the same run.

---

316-329: Consider adding meaningful assertions beyond smoke tests.

Several transition_membership_status() tests only assert $this->assertTrue(true) after calling the method. While this verifies no exceptions are thrown, consider adding assertions that verify the membership state wasn't unexpectedly modified (e.g., status unchanged when transition should be a no-op).

💡 Example enhancement for one test

 public function test_transition_membership_status_active_to_cancelled_returns_early(): void {
 
     $membership = $this->create_membership(['status' => Membership_Status::ACTIVE]);
     $manager    = $this->get_manager_instance();
+    $original_status = $membership->get_status();
 
     // This should return early because 'active' is not in allowed_previous_status.
     $manager->transition_membership_status(
         Membership_Status::ACTIVE,
         Membership_Status::CANCELLED,
         $membership->get_id()
     );
 
-    $this->assertTrue(true);
+    // Verify membership wasn't modified by a no-op transition
+    $refreshed = wu_get_membership($membership->get_id());
+    $this->assertEquals($original_status, $refreshed->get_status());
 }

Also applies to: 334-346, 351-363, 368-381, 386-399, 404-416, 421-433

tests/WP_Ultimo/Managers/Site_Manager_Test.php (1)

826-840: Consider cleaning up created sites in tests.

Multiple tests create sites via wu_create_site() but don't delete them afterward. While WordPress test framework may handle cleanup between test classes, explicit cleanup ensures test isolation and prevents potential side effects.

💡 Example cleanup pattern

 public function test_wu_create_site_creates_basic_site(): void {
 
     $site = wu_create_site(
         [
             'title'  => 'Basic Test Site',
             'domain' => 'basic-test.example.com',
             'path'   => '/',
         ]
     );
 
     $this->assertNotWPError($site);
     $this->assertInstanceOf(\WP_Ultimo\Models\Site::class, $site);
     $this->assertNotEmpty($site->get_id());
     $this->assertEquals('Basic Test Site', $site->get_title());
+
+    // Clean up
+    $site->delete();
 }

Also applies to: 845-871, 880-893, 898-911

tests/WP_Ultimo/Managers/Domain_Manager_Test.php (1)

1-10: Namespace inconsistency with other test files.

This test file uses namespace WP_Ultimo\Managers, but other test files in this PR (e.g., Membership_Manager_Test.php, Site_Manager_Test.php) use WP_Ultimo\Tests\Managers. Consider aligning the namespace for consistency.

♻️ Proposed fix

 <?php
 
-namespace WP_Ultimo\Managers;
+namespace WP_Ultimo\Tests\Managers;
 
-use WP_UnitTestCase;
+use WP_Ultimo\Managers\Domain_Manager;
 use WP_Ultimo\Settings;
 use WP_Ultimo\Models\Domain;
 use WP_Ultimo\Database\Domains\Domain_Stage;
 
-class Domain_Manager_Test extends WP_UnitTestCase {
+class Domain_Manager_Test extends \WP_UnitTestCase {

tests/WP_Ultimo/Models/Domain_Test.php (3)

1-15: Namespace inconsistency with other test files.

This test file uses namespace WP_Ultimo\Models, but other test files in this PR use WP_Ultimo\Tests\... namespacing. Consider aligning for consistency.

♻️ Proposed fix

 <?php
 
-namespace WP_Ultimo\Models;
+namespace WP_Ultimo\Tests\Models;
 
-use WP_UnitTestCase;
+use WP_Ultimo\Models\Domain;
 use WP_Ultimo\Database\Domains\Domain_Stage;
 
 /**
  * Test class for Domain model functionality.
  *
  * Tests SSL certificate validation for custom domains including
  * valid certificates, invalid certificates, and empty domain handling.
  * Also tests getters/setters, business logic, stage methods, and CRUD.
  */
-class Domain_Test extends WP_UnitTestCase {
+class Domain_Test extends \WP_UnitTestCase {

---

37-68: External network dependency in SSL tests may cause CI flakiness.

These tests make real network calls to validate SSL certificates (dogs.4thelols.uk and eeeeeeeeeeeeeeeeauauexample.com). This can cause intermittent failures due to network issues, DNS resolution, or certificate changes. Consider:

1. Mocking the SSL validation for unit tests
2. Marking these as integration tests to run separately
3. Using @group network annotation to skip in CI if needed

---

17-32: Consider adding tearDown() to clean up created blog.

The setUp() creates a test blog via self::factory()->blog->create() but there's no corresponding tearDown() to clean it up. While WordPress test framework may handle this, explicit cleanup ensures no side effects.

💡 Proposed addition

+	/**
+	 * Clean up test environment.
+	 */
+	public function tearDown(): void {
+
+		if ($this->blog_id) {
+			wp_delete_site($this->blog_id);
+		}
+
+		parent::tearDown();
+	}
+
 	/**
 	 * Test that has_valid_ssl_certificate returns true for valid SSL certificates.
 	 */